-
-
Notifications
You must be signed in to change notification settings - Fork 52
Implement chapter time over full audiobook duration #1090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4acc267 to
27df1b9
Compare
|
I fixed the possible undefined errors during build, note that this is the first time I ever touch TS and Vue, so let me know if there are more standard ways to do what I did 😅 |
|
Ready for another lint run/review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great enhancement! Just some small comments on non-null assertion
d78c4e6 to
205819d
Compare
205819d to
2b3f74b
Compare
|
Having just started to play with Audiobooks there are a number of considerations here. For audiobooks with not so many chapters having the full timeline means you can click on the chapter mark to jump to that chapter. Obviously in the example above this is not that relevant as there are so many chapters so maybe not something to worry about. However, while on this subject one thing the UI lacks is an indication of which chapter is currently playing. Is there anything we can do there? P.S. Check the lint probs... |
2b3f74b to
fad9aaa
Compare
|
Whoops indeed I pushed to the wrong branch again, pushed to the one I use for my own build, but forgot to also push to the MR one 🙈
Indeed, though out of scope for this MR, maybe next one :) |
|
Without showing the user that this is a chapter playing, this is going to cause unclarity. We need to figure out the UX how to display/select chapters. Silently only showing the current chapter makes the progress bar nicer maybe not perse more easier. |
At least for me, now it takes it from completely unusable and dangerous (misclick means I lose my place and jump to a random place in the book), to useful as I can seek around. I agree about the indication though - but I'd say this can be a first step to be merged as it's an opt-in and users should be aware that they have switched it on. I do intend to look at the second part after. |
|
You can just touch a chapter in the list to jump there |
cb33d2f to
302270f
Compare
|
I just added audiobook chapter subtitles, this should be better now. |
|
Have you got some screenshots? |
302270f to
08025d8
Compare
| const tempTime = ref(0); | ||
| const chapterTime = computed(() => | ||
| localStorage.getItem("frontend.settings.audiobook_chapter_time") == "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we certain a computed property is the right way to do this ?
Like, will it only be computed once and not over and over ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, have not used vue before this. I'm open to ideas.
On the other hand: Does it matter? How much overhead is a map lookup?
|
@marcelveldt This subtitle idea is somewhat aligned with the concept of providing another line in the now playing view to provide extra information for the Radio Paradise provider? Should we review that for a consolidated approach? |
666373f to
119fafc
Compare
|
@marcelveldt Ready for another review round |


Why? A picture is a thousand words: